-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf(ObjectStoreStorage): Improve (slow) move on same object bucket #47986
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs some tests to verify the behavior and prevent regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Changes committed :-).
21172bf
to
b11ab3c
Compare
Tests are still missing though. |
I hope, that the object store tests from https://github.com/nextcloud/server/tree/master/tests/lib/Files/ObjectStore will give us a hint, if this change causes harm. |
Sure there shouldn't be any regressions from this change, but we also need a test that ensures the new behavior is present. So please add a test that moves a file on an object storage and ensures this case is triggered. |
a025436
to
a700954
Compare
ddd2296
to
7c1a141
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! This is a really good improvement 💙
27984de
to
f724be3
Compare
You need to follow the conventional commit format, so the commit message should be something like |
f724be3
to
2639aa0
Compare
This does not only improve perfs but fixes a bug when you have an object storage as primary. In that case without this change, when object in unlinked it is deleted and as the object copied is the same than the original one, if object being moved is actually deleted. In the end you can see your moved objects in your nextcloud interface but it has been deleted in your object store. Please backport it, this a pretty dangerous bug, we had thousands of files being deleted (we can provide guidelines to restore it for those who also face this issue). |
/backport to stable28 |
I'm not convinced this needs to be backported to fix a bug. Are there any reports about it already? |
We have experienced the issue on multiple Nextcloud instances at IndieHosters. It could lead to data loss on buckets without versioning. So I think it's pretty serious. |
Like @unteem said, it is quite an impacting bug. |
But the file isn't just deleted from the S3. This change only skips deleting and uploading the same content again. |
We're from the same company. 3e12e1e introduced the idea of keeping the same file id and thus object key in s3. It theoretically works fine on different s3 storage because it copies the object first and then unlinks it from the source (delete it). |
CC @icewind1991 |
We also had tens of files removed, using v28.0.11. |
/backport to stable29 |
The problem is the tests added in this PR were broken (as all of our object storage tests weren't executed by CI it wasn't noticed) and @icewind1991 made some fixes, but they didn't get backported and there were so many changes I'm not sure what is needed to make it work. |
we could just remove the tests ? Do you want me to make a PR with just the fix and not the tests ? |
@unteem would be awesome if you could look into fixing the tests. The fixes are known, it is a matter of extracting them from follow-up PRs. Here are some pointers: |
The regression when moving files fixed by this pull request can be tested in the following way:
Before 9597072 an internal error is returned, and the logs show that the object could not be got from the storage. After 9597072 the contents of the file are returned as expected. However, the regression was not introduced in 3e12e1e, but in bd740ac. Therefore, through backports it was introduced in Nextcloud 30.0.0, Nextcloud 29.0.7 and Nextcloud 28.0.11. Backports to Nextcloud 27 and Nextcloud 26 are still open and not merged. |
It is the combination of 3e12e1e and how it is used in 3e12e1e that is causing the issue. Since this first one when object are copied the That is why this PR fixes it. |
/backport to stable28 |
The backport to # Switch to the target branch and update it
git checkout stable28
git pull origin stable28
# Create the new backport branch
git checkout -b backport/47986/stable28
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 9597072a
# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/47986/stable28 Error: No changes found in backport branch Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports. |
Isn't it already backported? |
only 30 got this change so far |
I tried to backport locally and the change is already on stable28. |
Summary
This commit fixes the issue #47856. When you upload a file into a group folder and when you use a single S3 bucket as primary storage, the final move operation hangs for a long time. In the background, Nextcloud initiates a copy-delete sequence from the bucket into the bucket, with causes a lot unnecessary overhead. Nextcloud thinks that the file must be imported to another storage and does not recognize that everything is done on the same object bucket. In that case, the import step can be completely skipped, which saves time, network bandwidth and reduces the load on the object storage.
The behavior improves a lot with #46013. However, after upload there are still some unnecessary put messages that are being sent to the object storage when you use an object storage as primary storage and upload files into a group folder.
Checklist